Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

target/sim: Add JTAG tasks for reg access and preloading #103

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

alex96295
Copy link
Collaborator

@alex96295 alex96295 commented Feb 14, 2024

First of three steps to merge back features/bugfixes from Carfield project. This is a testbench/simulation-only feature.

Remaining two steps (RTL features) are #102 and #74, to be addressed later

* Can be used by platforms to halt CVA6 and preload a shared memory when
execution happens on domains different than Cheshire.
@alex96295 alex96295 marked this pull request as ready for review February 14, 2024 10:35
@alex96295 alex96295 requested a review from paulsc96 February 14, 2024 10:35
@alex96295 alex96295 self-assigned this Feb 14, 2024
@alex96295 alex96295 added the enhancement New feature or request label Feb 14, 2024
@alex96295
Copy link
Collaborator Author

@paulsc96 rfc when you have time

@paulsc96
Copy link
Collaborator

Here are my comments:

  • Please stay consistent with code formatting. This file has no newlines in tasks, no trailing whitespaces in task parameter lists, and no spelling errors in comments, and all that is intentional :)
  • Please don't duplicate functionality.
    • jtag_elf_halt_load does an exact subset of what jtag_elf_run does, so jtag_elf_run should reuse this new code.
    • Same goes with jtag_poll_bit0 and jtag_read_reg32.
    • Same goes with jtag_elf_run and jtag_write_reg32.
  • Please name these meaningfully. If jtag_read_reg32 has the 32 suffix, why does jtag_read_reg, doing exclusively double reads, not have a 64 suffix? Why even hardcode access size if the difference in task code are so minimal?

Admittedly, I could have given this feedback earlier, but these "oversights" (read: you dumped this code here without a second look at the diff) discouraged me a bit. It's faster and simpler to do what you should have done and clean up the diff, but you knew I would anyways 😉 Good on you for dodging work at my expense.

@paulsc96
Copy link
Collaborator

paulsc96 commented Feb 23, 2024

Bonus round of comments (to be updated):

  • (Added on top for relevance) What you set the SBCS register to is important. Your jtag_write_reg32 task sets sbreadonaddr, but never reads after writing to SBAddress0, leaving dead data behind in the debug module that a subsequent read task would erroneously read. This is bad.
  • When checking writes, are you sure you can read immediately after writing? I am not sure anything is strictly enforcing read-write consistency on our debug path.
  • The tasks are intentionally ordered by their dependencies (i.e. tasks depending on others are defined after their dependencies). Really helps reading these tasks.
  • Why do your tasks hardcode 32 bit addresses? This is a 64-bit system with (potentially) up to 64 bits of address, and all other tasks use a doub_bt accordingly.
  • You have a congruent set of four tasks reading and writing 32 and 64 bit data, but they cannot agree on consistent naming of arguments, variables, and even types (more in this in the next point).
    • Wait, are these supposed to be congruent? They are all over the place... jtag_read_reg and jtag_read_reg32 differ only in their argument types in effect (either both 2-value 64-bit or 4-value 32-bit), while jtag_write_reg and jtag_write_reg32 have the same widths (but not value types?) in both cases and just truncate them? Also, jtag_write_reg checks the write and jtag_write_reg32 does not. If this PR is an elaborate joke, this is where you should tell me. In any case, I am laughing by now..
    • At this point, I am not sure these actually work as intended - but then again, I can't discern the intent, so you got me there. Why is sbaccess always 2 (32-bit accesses)?
  • 2-value and 4-value logic vectors are not the same, and the *_bt types are 2-value unlike the logic vectors you introduce (why? all other tasks of this kind use 2-value types).
  • Whether and when a task displays messages should be consistent. Why do the register write tasks print, but the read tasks don't?

@paulsc96
Copy link
Collaborator

Here's my proposal for something mergeable:

  • I integrated jtag_elf_halt_load into jtag_elf_run, as it was literally copy-pasted from that task to begin with.
  • I discarded jtag_read_reg and jtag_write_reg, since I simply cannot understand what they intended to do or how they are supposed to differ from the 32-suffixed variants. Those, in turn, I cleaned up a bit and addressed the above points. Since I make no 32-bit writes yet, there is no point in reusing the 32 variants specifically.

If you want the non-32-variant tasks back, they should at least do something clearly discernable.

@paulsc96 paulsc96 force-pushed the aottaviano/jtag branch 3 times, most recently from cfe3329 to 98750bd Compare February 23, 2024 20:19
@paulsc96 paulsc96 changed the title target/sim: Add jtag tasks to write/read 32b and 64b registers target/sim: Add JTAG tasks for reg access and preloading Feb 23, 2024
@paulsc96 paulsc96 merged commit f316617 into main Mar 5, 2024
18 checks passed
@paulsc96 paulsc96 deleted the aottaviano/jtag branch March 5, 2024 13:27
yvantor pushed a commit that referenced this pull request Mar 5, 2024
* target/sim: Add JTAG tasks to read/write 32b registers

* target/sim: Add JTAG task to halt and load binary

Can be used by platforms to halt CVA6 and preload a shared memory when
execution happens on domains different than Cheshire.

* target/sim: Clean up added tasks

---------

Co-authored-by: Paul Scheffler <[email protected]>
yvantor pushed a commit that referenced this pull request May 10, 2024
* target/sim: Add JTAG tasks to read/write 32b registers

* target/sim: Add JTAG task to halt and load binary

Can be used by platforms to halt CVA6 and preload a shared memory when
execution happens on domains different than Cheshire.

* target/sim: Clean up added tasks

---------

Co-authored-by: Paul Scheffler <[email protected]>
chaoqun-liang pushed a commit that referenced this pull request May 18, 2024
* target/sim: Add JTAG tasks to read/write 32b registers

* target/sim: Add JTAG task to halt and load binary

Can be used by platforms to halt CVA6 and preload a shared memory when
execution happens on domains different than Cheshire.

* target/sim: Clean up added tasks

---------

Co-authored-by: Paul Scheffler <[email protected]>
chaoqun-liang pushed a commit that referenced this pull request Jun 1, 2024
* target/sim: Add JTAG tasks to read/write 32b registers

* target/sim: Add JTAG task to halt and load binary

Can be used by platforms to halt CVA6 and preload a shared memory when
execution happens on domains different than Cheshire.

* target/sim: Clean up added tasks

---------

Co-authored-by: Paul Scheffler <[email protected]>
Aquaticfuller pushed a commit that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants